AttributeValueBase: include xs namespace definition in set_type#336
Closed
jgehrcke wants to merge 1 commit intoIdentityPython:masterfrom
Closed
AttributeValueBase: include xs namespace definition in set_type#336jgehrcke wants to merge 1 commit intoIdentityPython:masterfrom
jgehrcke wants to merge 1 commit intoIdentityPython:masterfrom
Conversation
Contributor
Author
|
Two things I noticed was:
Looks like this was thought about, but then postponed. @rohe would appreciate your feedback, thanks! |
Contributor
Author
|
I was a little surprised to see all CI tests pass. We should probably add a test that would have failed before. |
|
These changes look good to me. 👍 |
Contributor
Author
|
Closing, after #365 has been merged. Thanks for pushing and discussing @jaustinpage @ashimaathri @rohe |
Member
|
Yes, a failing test would have been nice, so that future changes do not break this edge case again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The following response generated by pysaml2 does not pass schema validation (against saml-schema-protocol-2.0.xsd):
Errors:
http://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf says that using
xsi:typeis optional:Now that we use it, we must also declare the xs namespace. Indeed, in test data used by pysaml2 we see that declaration (as an attribute in the
AttributeValuetag):Ref: https://github.com/rohe/pysaml2/blob/master/tests/attribute_statement_data.py#L81
In the SimpleSamlPHP auth response we see that the namespace is declared at the document root:
Ref: https://github.com/rohe/pysaml2/blob/master/tests/simplesamlphp_authnresponse.xml#L3
My intial goal was to also get the declaration added to the document root if required (not just always). However, my deepdive into pysaml2's XML foo was not deep enough.
Still, I could come up with a (hopefully clean and nice) solution to conditionally add the xs namespace declaration to
AttributeValuetags in anAttributeStatement. That's also how it's still documented for simplesamlphp: https://simplesamlphp.org/docs/1.11/simplesamlphp-metadata-extensions-attributes.I've grepped the code base and it seems that
set_type()is consistently used -- so it's the central entry point forxs:-based types into the document. That's why it seems to be a proper place to conditionally add the namespace declaration.The patch works, in the sense that the resulting documents contain the declaration and pass the validation that failed previously.
This issue has actually been reported before:
This PR was mentioned as related: #326 -- I am not sure if that works, and the changes are probably way larger than required.
Related: